Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

replaced AlertDialog without AppCompatAlertDialog #924

Closed
wants to merge 2 commits into from
Closed

replaced AlertDialog without AppCompatAlertDialog #924

wants to merge 2 commits into from

Conversation

StefMa
Copy link
Contributor

@StefMa StefMa commented Oct 11, 2015

See #818 too

@StefMa
Copy link
Contributor Author

StefMa commented Oct 11, 2015

#896 well :( Should not be merged right?

@@ -54,4 +54,9 @@
<item name="colorAccent">@color/accent</item>
</style>

<style name="Theme.GitHub.Alert.Dialog.Style" parent="Theme.AppCompat.Light.Dialog.Alert">
<item name="colorAccent">@color/accent</item>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are you setting colorAccent to @color/accent again? its already set in the main theme

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah. You are right :-)

@larsgrefer
Copy link
Contributor

@StefMa I'd like to use appcompats AlertDialogs, but @hzsweers and @Meisolsson don't

<style name="Theme.GitHub.Alert.Dialog.Style" parent="Theme.AppCompat.Light.Dialog.Alert">
<item name="android:textColorPrimary">@color/accent</item>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would be the default value for android:textColorPrimary at this point?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Depends on your main style. With our GitHub . Light it is black...

@StefMa
Copy link
Contributor Author

StefMa commented Oct 11, 2015

Ok, let us discuss 👍
@Meisolsson want to stay on the MaterialDialogLib without a reason.
@hzsweers want to stay on this lib because of:

We should be moving all the dialogs to material dialogs then. There are
features of it that don't exist in the appcompat's dialogs as well, such as
sensible widths on tablets and more customizability. I don't know who did
the original addition of it here, but I very much prefer them to
appcompat's.

So currently we don't need the "more features" which the MaterialDialog-Lib brings on. So, actually it is only overhead on source code.
Also, but this is my opinion, we don't need "more features" on dialogs. Dialogs are only "simple" information "things".
According to Design Guid Dialogs are:

Dialogs inform users about critical information, require users to make decisions, or encapsulate multiple tasks within a discrete process. Use dialogs sparingly because they are interruptive in nature. Their sudden appearance forces users to stop their current task and refocus on the dialog content. Not every choice, setting, or detail warrants interruption and prominence.

@ZacSweers
Copy link
Contributor

So currently we don't need the "more features" which the MaterialDialog-Lib brings on. So, actually it is only overhead on source code.

Run the app on a tablet and suddenly we do.

Look, this is not a discussion, we're not going to switch to it. It's not a discussion because we have already discussed it. In the future, please abide by the rules we've laid out in our CONTRIBUTING.md about infrastructure changes like this and discuss with us first before assuming anything. Especially considering this was already discussed and decided before.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants